Repository navigation
FG Manager - #524
FG Manager#524
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds foreground dialog support: new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (bg task)
participant BgMgr as BgManager
participant Updater as Updater
participant Handler as UpdateHandler
participant FgMgr as DialogManager
Caller->>BgMgr: call bg(...).fg() (await)
BgMgr->>BgMgr: create DialogFgEvent(entered, exited)
BgMgr->>Updater: notify_task(DialogUpdate(event=FG))
Updater->>Updater: schedule _process_update (create_task)
BgMgr->>BgMgr: await event.entered
Updater->>Handler: _process_update -> handle_update
Handler->>Handler: detect DialogFgEvent
Handler->>FgMgr: set event.entered.result(dialog_manager)
Handler->>Handler: await event.exited
BgMgr-->>Caller: yield DialogManager (context entered)
Caller->>FgMgr: perform foreground actions (update/start/...)
Caller-->>BgMgr: exit context -> BgMgr sets event.exited (None or exception)
Handler->>Handler: resume after awaited exited
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_fg.py (1)
33-44: Duplicate handler implementations provide no additional test coverage.
start_for_second_userandstart_for_second_user_via_bgare identical. Consider consolidating into a single handler and reusing it across tests, or if the intention is to test different scenarios, make the implementations actually differ.♻️ Proposed consolidation
async def start_for_second_user( event: Any, dialog_manager: DialogManager, ) -> None: async with dialog_manager.bg(user_id=2, chat_id=-1).fg() as manager: await manager.start(MainSG.start, mode=StartMode.RESET_STACK) - -async def start_for_second_user_via_bg( - event: Any, dialog_manager: DialogManager, -) -> None: - async with dialog_manager.bg(user_id=2, chat_id=-1).fg() as manager: - await manager.start(MainSG.start, mode=StartMode.RESET_STACK)Then reuse
start_for_second_userin both tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_fg.py` around lines 33 - 44, Two test handlers are identical: start_for_second_user and start_for_second_user_via_bg; consolidate by keeping a single implementation (e.g., start_for_second_user) and remove the duplicate implementation start_for_second_user_via_bg, then update any tests that referenced start_for_second_user_via_bg to call start_for_second_user instead; alternatively, if distinct behavior was intended, modify start_for_second_user_via_bg to implement the different scenario (so the function bodies differ). Ensure you update all references to the removed function name in tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/aiogram_dialog/manager/bg_manager.py`:
- Around line 263-283: The fg() asynccontextmanager currently creates futures
with asyncio.get_event_loop() and then calls event.as_(bot) which
serializes/reconstructs the event (losing the original event.entered/exited
futures), uses the wrong return type, and can hang if notify_task fails; fix by:
change the return annotation of fg to AsyncIterator[DialogManager]; replace
asyncio.get_event_loop().create_future() with
asyncio.get_running_loop().create_future() for both event.entered and
event.exited; do not call event.as_(bot) (i.e. pass the original DialogFgEvent
instance into DialogUpdate/notify_task so the same futures are used) and
schedule notify_task as a Task, then await completion using
asyncio.wait({event.entered, task}, return_when=FIRST_COMPLETED) so you can
detect if notify_task finished with an exception (raise it) or if event.entered
completed; keep the existing exited.set_result/exited.set_exception handling and
ensure you await the notify task after the context exit.
In `@src/aiogram_dialog/manager/update_handler.py`:
- Around line 31-33: The DialogFgEvent handshake can raise InvalidStateError
because event.entered may be cancelled or already resolved; in the DialogFgEvent
branch inside update_handler (the block that calls
event.entered.set_result(dialog_manager) and awaits event.exited) guard the
future before setting it by checking its state (e.g., only call set_result if
not event.entered.done() and not event.entered.cancelled()), or wrap set_result
in a try/except InvalidStateError and ignore the exception, so replace the
unconditional set_result call with a guarded check or safe try/except to avoid
breaking the update flow.
---
Nitpick comments:
In `@tests/test_fg.py`:
- Around line 33-44: Two test handlers are identical: start_for_second_user and
start_for_second_user_via_bg; consolidate by keeping a single implementation
(e.g., start_for_second_user) and remove the duplicate implementation
start_for_second_user_via_bg, then update any tests that referenced
start_for_second_user_via_bg to call start_for_second_user instead;
alternatively, if distinct behavior was intended, modify
start_for_second_user_via_bg to implement the different scenario (so the
function bodies differ). Ensure you update all references to the removed
function name in tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85eb56f7-526b-4149-b110-f50fb1ca6f64
📒 Files selected for processing (10)
example/loading.pysrc/aiogram_dialog/api/entities/update_event.pysrc/aiogram_dialog/api/protocols/manager.pysrc/aiogram_dialog/manager/bg_manager.pysrc/aiogram_dialog/manager/manager.pysrc/aiogram_dialog/manager/sub_manager.pysrc/aiogram_dialog/manager/update_handler.pysrc/aiogram_dialog/manager/updater.pysrc/aiogram_dialog/tools/preview.pytests/test_fg.py
| @asynccontextmanager | ||
| async def fg(self) -> Iterator[DialogManager]: | ||
| event = DialogFgEvent( | ||
| data=None, | ||
| action=DialogAction.FG, | ||
| entered=asyncio.get_event_loop().create_future(), | ||
| exited=asyncio.get_event_loop().create_future(), | ||
| **self._base_event_params(), | ||
| ) | ||
| bot = self._event_context.bot | ||
| update = DialogUpdate(aiogd_update=event.as_(bot)).as_(bot) | ||
| task = self._updater.notify_task(bot=bot, update=update) | ||
| manager = await event.entered | ||
| try: | ||
| yield manager | ||
| except Exception as e: | ||
| event.exited.set_exception(e) | ||
| raise e | ||
| else: | ||
| event.exited.set_result(None) | ||
| await task |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how DialogFgEvent and DialogUpdate handle as_() and whether Future fields are preserved
echo "=== Check DialogFgEvent definition ==="
ast-grep --pattern $'class DialogFgEvent($_):
$$$
'
echo "=== Check DialogUpdate definition ==="
ast-grep --pattern $'class DialogUpdate($_):
$$$
'
echo "=== Check if as_() is overridden in these classes ==="
rg -n "def as_\(" --type py -C 3Repository: Tishka17/aiogram_dialog
Length of output: 1450
🌐 Web query:
Python asyncio.get_event_loop deprecation asyncio.get_running_loop
💡 Result:
asyncio.get_event_loop is deprecated in scenarios where no current event loop is set, with changes escalating across Python versions. In Python 3.10-3.11, it emitted DeprecationWarning when no running loop existed. Since Python 3.12, it warns when no running or current loop is set. In Python 3.14 (current as of 2026-03-21), it raises RuntimeError if no current event loop exists. The asyncio policy system, which supported implicit loop creation, is deprecated since 3.14 and slated for removal in 3.16. The recommended replacement is asyncio.get_running_loop, which returns the running event loop in the current thread but raises RuntimeError if no loop is running. It can only be called from coroutines or callbacks and has simpler, more predictable behavior. Key differences: - get_event_loop: Complex logic; falls back to policy if no running loop; now errors without set loop. - get_running_loop: Strictly returns running loop or errors; preferred in async contexts. For code needing a loop outside running contexts (e.g., synchronous code), use asyncio.new_event_loop explicitly, set it with asyncio.set_event_loop, and manage its lifecycle (run_until_complete, shutdown, close). Prefer high-level asyncio.run for top-level entry points. Example migration in async context: # Old (deprecated) loop = asyncio.get_event_loop # New loop = asyncio.get_running_loop Outside async: # Old (now errors in 3.14) loop = asyncio.get_event_loop # New loop = asyncio.new_event_loop asyncio.set_event_loop(loop) try: loop.run_until_complete(coro) finally: loop.run_until_complete(loop.shutdown_asyncgens) loop.close Or better, use asyncio.run(coro).
Citations:
- 1: https://docs.python.org/3.11/library/asyncio-eventloop.html
- 2: https://docs.python.org/3/library/asyncio-eventloop.html
- 3: https://stackoverflow.com/questions/65206110/when-to-use-asyncio-get-running-loop-vs-asyncio-get-event-loop
- 4: https://docs.python.org/3.14/library/asyncio-eventloop.html
- 5: https://docs.python.org/3.14/library/asyncio-policy.html
- 6: https://docs.python.org/3/library/asyncio-policy.html
- 7: https://docs.python.org/3/deprecations/pending-removal-in-3.16.html
🏁 Script executed:
#!/bin/bash
# Check parent class of DialogUpdateEvent
echo "=== DialogUpdateEvent parent class ==="
rg -n "class DialogUpdateEvent" -A 1 --type py
# Check Update class and as_() implementation
echo "=== Update class from aiogram ==="
rg -n "class Update" -A 5 --type py | head -50
# Check if DialogFgEvent or DialogUpdateEvent override as_()
echo "=== DialogFgEvent and DialogUpdateEvent implementations ==="
rg -n "class DialogFgEvent\|class DialogUpdateEvent" -A 20 --type py src/aiogram_dialog/api/entities/update_event.py
# Check the fg() method return type annotation
echo "=== fg() method definition ==="
rg -n "async def fg" -B 1 -A 2 --type py src/aiogram_dialog/manager/bg_manager.pyRepository: Tishka17/aiogram_dialog
Length of output: 1019
🏁 Script executed:
#!/bin/bash
# Search for as_() method implementation in TelegramObject or base classes
echo "=== Search for as_() method implementations ==="
rg -n "def as_\(" -B 2 -A 10 --type py | head -100
# Check if DialogUpdateEvent or DialogFgEvent has field exclusion or special handling
echo "=== Check for field handling in update_event.py ==="
cat src/aiogram_dialog/api/entities/update_event.py | head -100Repository: Tishka17/aiogram_dialog
Length of output: 1657
🏁 Script executed:
#!/bin/bash
# Check for TelegramObject import and search in dependencies
echo "=== Check if TelegramObject is from aiogram ==="
rg -n "from aiogram" src/aiogram_dialog/api/entities/update_event.py
# Look for handle_update implementation in bg_manager.py
echo "=== Search for handle_update in bg_manager ==="
rg -n "def handle_update\|event\.entered\|event\.exited" --type py src/aiogram_dialog/manager/bg_manager.py
# Search for how the event is processed in the update handler
echo "=== Search for DialogFgEvent handling ==="
rg -n "DialogFgEvent" --type py -B 2 -A 5
# Check the notify_task method to understand the flow
echo "=== Search for notify_task ==="
rg -n "def notify_task\|notify_task" --type py src/aiogram_dialog/manager/bg_manager.py -A 10 | head -60Repository: Tishka17/aiogram_dialog
Length of output: 3365
🏁 Script executed:
#!/bin/bash
# Check project's Python version requirements
echo "=== Check Python version requirements ==="
fd -e "pyproject.toml" -o -e "setup.py" -o -e "setup.cfg" | xargs cat 2>/dev/null | grep -i "python" | head -20
# Check if get_running_loop is already used anywhere in the project
echo "=== Check for get_running_loop usage ==="
rg -n "get_running_loop" --type py | head -20Repository: Tishka17/aiogram_dialog
Length of output: 403
🏁 Script executed:
#!/bin/bash
# Check if TelegramObject.as_() serializes to dict
echo "=== Check TelegramObject import and understand model behavior ==="
rg -n "TelegramObject\|model_dump\|model_validate" src/aiogram_dialog/api/entities/update_event.py
# Check the actual flow - how is the event passed through the system
echo "=== Check how update is created and passed ==="
sed -n '270,280p' src/aiogram_dialog/manager/bg_manager.py
# Check notify_task signature
echo "=== Check notify_task method signature ==="
rg -n "def notify_task" src/aiogram_dialog/manager/updater.py -A 5
# Check if the event object is reused or reconstructed
echo "=== Check if event is reconstructed from update ==="
rg -n "aiogd_update\|event =" src/aiogram_dialog/manager/update_handler.py -B 2 -A 2 | head -40Repository: Tishka17/aiogram_dialog
Length of output: 966
Critical: Future references are lost during event serialization, causing indefinite hang.
When event.as_(bot) is called on line 273, the event is serialized and reconstructed. The DialogFgEvent object passed to notify_task is no longer the same instance with the live entered and exited Future objects created on lines 268-269. When the update handler receives this reconstructed event and attempts event.entered.set_result(dialog_manager) (update_handler.py:32), it sets the result on a different Future instance than the one being awaited on line 276, causing an indefinite hang.
Additionally:
- Type annotation: Return type should be
AsyncIterator[DialogManager], notIterator[DialogManager](line 264). - Deprecation:
asyncio.get_event_loop()is deprecated in Python 3.10+ and raisesRuntimeErrorin Python 3.14+. Useasyncio.get_running_loop()instead (lines 268-269). - Error handling: If
notify_taskfails or is cancelled beforeevent.enteredis set,await event.enteredwill hang indefinitely with no timeout or cancellation mechanism.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/aiogram_dialog/manager/bg_manager.py` around lines 263 - 283, The fg()
asynccontextmanager currently creates futures with asyncio.get_event_loop() and
then calls event.as_(bot) which serializes/reconstructs the event (losing the
original event.entered/exited futures), uses the wrong return type, and can hang
if notify_task fails; fix by: change the return annotation of fg to
AsyncIterator[DialogManager]; replace asyncio.get_event_loop().create_future()
with asyncio.get_running_loop().create_future() for both event.entered and
event.exited; do not call event.as_(bot) (i.e. pass the original DialogFgEvent
instance into DialogUpdate/notify_task so the same futures are used) and
schedule notify_task as a Task, then await completion using
asyncio.wait({event.entered, task}, return_when=FIRST_COMPLETED) so you can
detect if notify_task finished with an exception (raise it) or if event.entered
completed; keep the existing exited.set_result/exited.set_exception handling and
ensure you await the notify task after the context exit.
| elif isinstance(event, DialogFgEvent): | ||
| event.entered.set_result(dialog_manager) | ||
| await event.exited |
There was a problem hiding this comment.
Guard foreground handshake futures before setting results.
At Line 32, set_result() is unconditional. If event.entered was cancelled by the waiting task, this can raise InvalidStateError and break the update flow.
💡 Proposed fix
elif isinstance(event, DialogFgEvent):
- event.entered.set_result(dialog_manager)
+ if event.entered.cancelled():
+ return
+ if not event.entered.done():
+ event.entered.set_result(dialog_manager)
await event.exited🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/aiogram_dialog/manager/update_handler.py` around lines 31 - 33, The
DialogFgEvent handshake can raise InvalidStateError because event.entered may be
cancelled or already resolved; in the DialogFgEvent branch inside update_handler
(the block that calls event.entered.set_result(dialog_manager) and awaits
event.exited) guard the future before setting it by checking its state (e.g.,
only call set_result if not event.entered.done() and not
event.entered.cancelled()), or wrap set_result in a try/except InvalidStateError
and ignore the exception, so replace the unconditional set_result call with a
guarded check or safe try/except to avoid breaking the update flow.
Summary by CodeRabbit
New Features
Tests
Documentation
Other